Skip to content

Ported GridEngine terabyte fix and shm processors from JobTree (resolves #409) - #454

Merged
hannes-ucsc merged 1 commit into
DataBiosphere:masterfrom
nsjake:master
Oct 9, 2015
Merged

Ported GridEngine terabyte fix and shm processors from JobTree (resolves #409)#454
hannes-ucsc merged 1 commit into
DataBiosphere:masterfrom
nsjake:master

Conversation

@nsjake

@nsjake nsjake commented Oct 5, 2015

Copy link
Copy Markdown

Modified init in MemoryString class in gridengine.py to support for terabyte (T) MEMTOT output from host, and specified shm processors via -pe shm in prepareQsub function.

Resolves #409

@cgcloud-jenkins

Copy link
Copy Markdown

Can one of the admins verify this patch?

@hannes-ucsc

Copy link
Copy Markdown
Member

The title of the PR should reference the issue it is addressing. Check out the other PRs for an example. Commit messages should start with upper-case letter. And please squash the two commits into one using interactive rebase. Then take off the needs work label from this PR.

@hannes-ucsc

Copy link
Copy Markdown
Member

Also, "ported changes from jobTree" is not descriptive enough for a commit message. Remember that most UIs only show the first 80 characters of the commit message so that's where you communicate the intend of the commit.

@nsjake nsjake changed the title ported changes from jobTree Added Support For Terabyte Output And shm Processors Oct 5, 2015
@nsjake

nsjake commented Oct 5, 2015

Copy link
Copy Markdown
Author

I made the changes! But I don't think I have permission to remove the "needs work" label myself.

@hannes-ucsc

Copy link
Copy Markdown
Member

Ah yes. Once we add you to the GH team, you will be able to.

@hannes-ucsc

Copy link
Copy Markdown
Member

Jenkins, ok to test.

@hannes-ucsc

Copy link
Copy Markdown
Member

The title of the PR should reference the issue it is addressing. Check out the other PRs for an example.

@nsjake nsjake changed the title Added Support For Terabyte Output And shm Processors Ported GridEngine terabyte fix And shm Processors From Jobtree Oct 6, 2015
@nsjake

nsjake commented Oct 6, 2015

Copy link
Copy Markdown
Author

Is this better?

@hannes-ucsc

Copy link
Copy Markdown
Member

No. Watch me ;-)

@hannes-ucsc hannes-ucsc changed the title Ported GridEngine terabyte fix And shm Processors From Jobtree Ported GridEngine terabyte fix and shm processors from JobTree (resolves #409) Oct 6, 2015
@hannes-ucsc

Copy link
Copy Markdown
Member

Now, if I merge the PR, the associated issue will be resolved, and it will reference the commit that resolves it. This kind of tracking is very important on a project with many people contributing.

@hannes-ucsc

Copy link
Copy Markdown
Member

Ah, I didn't notice this until now: the mode of the Makefile shouldn't be changed. There is no need for the Makefile to be executable. So that change needs to be eliminated and the commits squashed again.

@hannes-ucsc

Copy link
Copy Markdown
Member

Sorry, commit still includes mode change for Makefile.

@nsjake

nsjake commented Oct 7, 2015

Copy link
Copy Markdown
Author

Not sure what I did wrong the first time, but it should be fixed now!

@hannes-ucsc

Copy link
Copy Markdown
Member

Could you do some research on the difference between the smp and shm virtual environments and link to the corresponding web pages here? The author of benedictpaten/jobTree#41 had a problem with -l but in Toil someone already eliminated the -l switch and used the -pe switch instead.

@nsjake

nsjake commented Oct 7, 2015

Copy link
Copy Markdown
Author

"PE for Multithreaded or Shared Memory Parallel
If you have a multithreaded or shared-memory parallel job, i.e. it must run on a single node, you must use the "shm" parallel environment:

#$ -pe shm 16

If your program requires a number of slots which may fit on a single node, use the "shm" PE."

https://proteusmaster.urcf.drexel.edu/urcfwiki/index.php/Writing_Job_Scripts

"The smp parallel environment is designed to give you access to multiple cores on each machine. If your program is multi-threaded, and you want it to have 2 cores, you might run it this way:

% qsub -pe smp 2 runme
That will ensure that the process gets two job slots on each machine on which it runs. GridEngine will also ensure that twice the memory (if you requested memory) is available."

https://cs.brown.edu/about/system/services/hpc/gridengine/

"VMware Virtual SMP makes it possible for a single virtual machine to span up to four physical processors, or CPUs. These processors share the same memory, and work on any task regardless of the location of the task in memory."

https://www.vmware.com/pdf/vsmp_datasheet.pdf

"A shared memory is an extra piece of memory that is attached to some address spaces for their owners to use. As a result, all of these processes share the same memory segment and have access to it."

http://www.csl.mtu.edu/cs4411.ck/www/NOTES/process/shm/what-is-shm.html

Other links that might be helpful:

http://man7.org/linux/man-pages/man7/cpuset.7.html
https://en.wikipedia.org/wiki/Symmetric_multiprocessing
http://www.cyberciti.biz/tips/what-is-devshm-and-its-practical-usage.html

@hannes-ucsc

Copy link
Copy Markdown
Member

Thank you. That's helpful. We'll use shm. But we'll also make it configurable. Could you add code that uses the value of the TOIL_GRIDENGINE_PE environment variable if set and non-empty or falls back to shm otherwise?

@nsjake

nsjake commented Oct 7, 2015

Copy link
Copy Markdown
Author

Should I squash this with the other commit?

@cgcloud-jenkins

Copy link
Copy Markdown

Yes, that'd be great.

On Wed, Oct 7, 2015 at 2:11 PM, JakeNarkizian notifications@github.com
wrote:

Should I squash this with the other commit?


Reply to this email directly or view it on GitHub
#454 (comment).

You received this message because you are subscribed to the Google Groups
"cgloud-jenkins" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to cgloud-jenkins+unsubscribe@googlegroups.com.
To post to this group, send email to cgloud-jenkins@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/cgloud-jenkins/BD2KGenomics/toil/pull/454/c146330566%40github.com
https://groups.google.com/d/msgid/cgloud-jenkins/BD2KGenomics/toil/pull/454/c146330566%40github.com?utm_medium=email&utm_source=footer
.

For more options, visit https://groups.google.com/d/optout.

Hannes Schmidt
Software Engineer
Center for Biomolecular Science and Engineering
University of California, Santa Cruz

(206) 696-2316 (cell)
hannes@ucsc.edu

@hannes-ucsc

Copy link
Copy Markdown
Member

Not sure why the last post appears to come from Jenkins but it was me. I replied by email instead of using the GitHub web UI.

Modified init in MemoryString class in gridengine.py to support for terabyte (T) MEMTOT output from host, and specified shm processors via -pe shm in prepareQsub function. GridEngine Parallel Environment is now configurable, “shm” is the default value unless TOIL_GRIDENGINE_PE environment
variable is set and non-empty
@nsjake nsjake removed the needs work label Oct 8, 2015
@hannes-ucsc

Copy link
Copy Markdown
Member

Pretty sure the test failure is unrelated to this change. BTW, @JakeNarkizian are you able to access our Jenkins instance?

http://jenkins.cgcloud.info/

@hannes-ucsc

Copy link
Copy Markdown
Member

Jenkins, test this please.

@nsjake

nsjake commented Oct 8, 2015

Copy link
Copy Markdown
Author

Yes! Just got onto it now.

@hannes-ucsc

Copy link
Copy Markdown
Member

Jenkins, test this please.

@nsjake

nsjake commented Oct 9, 2015

Copy link
Copy Markdown
Author

Should I merge the pull request?

@hannes-ucsc

Copy link
Copy Markdown
Member

Only the build master (me) should merge PRs.

hannes-ucsc added a commit that referenced this pull request Oct 9, 2015
Ported GridEngine terabyte fix and shm processors from JobTree (resolves #409)
@hannes-ucsc
hannes-ucsc merged commit 5531bfa into DataBiosphere:master Oct 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants